-
Notifications
You must be signed in to change notification settings - Fork 282
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix CI and make libcurl a hard-dependency when using libobjc2 #447
Conversation
Seems like lld-link is once again discarding objc sections...
|
@qmfrederik any idea why we're getting those linker errors now with MSVC and the MinGW errors? |
@triplef The msvc job started failing when the NSURLSession rewrite (#422) PR got merged. This is the PR that introduced the The link failure may be related to llvm/llvm-project#49025. There's a workaround (see llvm/llvm-project#49025 (comment)) which probably works in this case, too? It'd be a matter of adding this:
to As a sidenote, I find that CI is most likely to be consistently green when all changes must go through pull requests, and CI is required to pass before a pull request can be merged. There's a fair amount of commits which have been pushed to the master branch without going through the PR process, and ended up breaking CI in one shape or form. |
Never worked for me, but I'll try :) |
I don't understand why the linker discards the Objective-C sections. There should be enough Objective-C code in the unit tests... It builds with a newer version of lld-link too. Edit: But this is only the case when |
CI in #422 passed for all targets except MiNGW GCC which was broken at that time: |
That's odd. Perhaps something subtle changed in a new version of LLVM, which exposes this? It's a bit difficult to analyze CI failures in this project because CI often fails. I think it's easier to do root cause analysis of build failures in projects which consistently use CI and require builds to be CI.
Looks like it did work this time? 😄 Really happy to see CI gradually becoming greener, thanks! |
Seems like we still have a deadlock in the NSThread test on MinGW GCC |
Good catch. But should this not be the case for the MSVC CI? |
Yes but the CI decided it no longer wants to use libcurl ^^
|
I believe that libcurl is currently a 'soft' dependency, i.e. libs-base will just build without curl support if it didn't find it. Perhaps we should upgrade that to a 'hard' dependency, and forcing users to pass an explicit (Sorry for the CI rants ^^) |
The only API that uses libcurl internally is NSURLSession which is only available when using the clang toolchain. Making it a hard dependency on GCC does not make sense. So perhaps a hard-dependency when using the libobjc2/clang toolchain? |
Took some time to install the MinGW environment on poor ICE Wifi, but the culpit for the deadlock is the |
…r is Redstone 5 (1809)
Everything except MinGW GCC works now. I cannot reproduce the deadlock and cannot get any logs out of the CI, as we use a test suite (a.k.a spaghetti shell code) that is unable to restrict the runtime of individual unit tests -_- |
@qmfrederik can you take a look at |
I'd suggest turn the CI green first, and address the spurious failures in NSThread Otherwise, we continue merging PRs blindly without proper testing. |
The build system is really fragile.
if "${CC}" -v 2>&1 | grep -q 'clang version'
does not work when theCC
variable consists of more than one word, as it is treated as the executable.Additionally, Win32 Native Threads was enabled on MinGW when running GCC which caused a build failure.